Skip to content

Conversation

durran
Copy link
Member

@durran durran commented Jan 13, 2025

Adds section to auth spec and CSFLE spec on allowing user provided custom credential providers for AWS and notes on testing.

Node implementation: mongodb/node-mongodb-native#4373

  • Update changelog.
  • Test changes in at least one language driver.
  • Test these changes against all server versions and topologies (including standalone, replica set, sharded
    clusters, and serverless).

@durran durran force-pushed the DRIVERS-2903 branch 3 times, most recently from e4b7aa5 to 00dbb01 Compare January 13, 2025 13:31
@durran durran force-pushed the DRIVERS-2903 branch 2 times, most recently from 55956fe to 51081d5 Compare January 29, 2025 14:08
@durran durran marked this pull request as ready for review January 29, 2025 14:35
@durran durran requested a review from a team as a code owner January 29, 2025 14:35
@durran durran requested review from JamesKovacs and blink1073 and removed request for a team January 29, 2025 14:35
@blink1073 blink1073 changed the title feat(DRIVERS-2983): use custom aws configuration feat(DRIVERS-2903): use custom aws configuration Jan 30, 2025
Copy link
Contributor

@jyemin jyemin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few quick edits while I perused this.

Copy link
Contributor

@jyemin jyemin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this needs to be supported in FLE as well, I think we need to update the FLE spec and specify how to configure the "aws" kms provider. Consider adding a similar property with same semantics to AWSKMSOptions in https://github.com/mongodb/specifications/blob/master/source/client-side-encryption/client-side-encryption.md#kmsproviders.

(While in some drivers you might be able to apply an auth mechanism property to FLE, in Java that isn't possible as auth mechanism properties are associated with a credential, not the MongoClient, and the credential of the MongoClient might not even be an AWS credential (or could be a different one).

@durran
Copy link
Member Author

durran commented Jan 30, 2025

Since this needs to be supported in FLE as well, I think we need to update the FLE spec as well and specify how to configure the "aws" kms provider. Consider adding a similar property with same semantics to AWSKMSOptions in https://github.com/mongodb/specifications/blob/master/source/client-side-encryption/client-side-encryption.md#kmsproviders.

(While in some drivers you might be able to apply an auth mechanism property to FLE, in Java that isn't possible as auth mechanism properties are associated with a credential, not the MongoClient, and the credential of the MongoClient might not even be an AWS credential (or could be a different one).

@kevinAlbs Do you have thoughts on this? Would there be a case where auto-encryption could want a different AWS credential provider than the associated MongoClient. (With ClientEncryption this is already possible since it has its own MongoClient)

@baileympearson
Copy link
Contributor

@durran What about users who aren't using AWS authentication for their clients but want to use AWS automatic KMS credential fetching with a custom provider? I don't think Node would support that without a provider option for KMS specifically.

1 similar comment
@baileympearson
Copy link
Contributor

@durran What about users who aren't using AWS authentication for their clients but want to use AWS automatic KMS credential fetching with a custom provider? I don't think Node would support that without a provider option for KMS specifically.

@durran
Copy link
Member Author

durran commented Jan 30, 2025

@durran What about users who aren't using AWS authentication for their clients but want to use AWS automatic KMS credential fetching with a custom provider? I don't think Node would support that without a provider option for KMS specifically.

That's a good point and I think a valid use case. I'll update everything accordingly.

@durran durran requested a review from a team as a code owner February 19, 2025 00:14
@durran durran requested review from jyemin and katcharov and removed request for a team February 19, 2025 00:14
@blink1073
Copy link
Member

cc @kevinAlbs since this affects Client Encryption options

@kevinAlbs kevinAlbs self-requested a review March 3, 2025 15:59
@durran durran requested a review from baileympearson March 10, 2025 20:42
Copy link
Contributor

@baileympearson baileympearson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, two minor clarifying comments. I'm not an auth or FLE spec owner though.

@durran
Copy link
Member Author

durran commented Mar 11, 2025

Pinging @jyemin as Java has an interest here. @kevinAlbs as I've added FLE options for the custom credential providers.

Copy link
Member

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Contributor

@jyemin jyemin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how we will define the types in the Java driver, as applications are allowed to use either AWS SDK v1 or v2, and those dependencies are optional so the types can't be part of the API. But we will figure out a way to make it work.

Copy link
Contributor

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Suggested wording changes, but nothing behavior changing.

@durran durran merged commit 57435eb into master Mar 12, 2025
5 checks passed
@durran durran deleted the DRIVERS-2903 branch March 12, 2025 22:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants